Visibility for retained gizmos#22053
Visibility for retained gizmos#22053blamelessgames wants to merge 24 commits intobevyengine:mainfrom
Conversation
- set `Visibility` as a required component on `Gizmo` components in `GizmoRenderPlugin` - update `extract_linegizmos` to take view visibility into account - update the 3d_gizmos example to toggle visibility of the retained gizmo in the scene investigating the feasibility of attaching an AABB to the gizmo for better visibility testing will build a `retained_gizmos` example to better demonstrate the feature. it's kind of crammed into the current example code
- add a visibility class to the gizmo - calculate AABBs for the gizmos - start a new example for retained gizmos
blamelessgames
force-pushed
the
visibility-for-retained-gizmos
branch
from
4917325 to
d07ee96
Compare
mnmaita
added
C-Feature
|
it occurs to me that it is probably possible to make the |
I think this is feasible but it lends itself to a bit of a scope expansion because there are at least two different ways this is currently happening, possibly more I haven't found yet. I'll address it in another more targeted PR |
kfc35
left a comment
kfc35
left a comment
There was a problem hiding this comment.
I checked out your branch and ran your example and it works as intended. The logic looks good from what I understand about gizmos and visibility culling, but I’m not a visibility culling expert so there may be a detail I missed.
I mostly have comments about the example that I think could be used to give more help to new users.
An aside about testing: how exactly did you use the new example to test that the visibility culling logic is working as expected? Since I assume if the camera looks away, I’m not sure if I can detect a difference between the gizmo becoming not visible versus staying visible off camera? But this may just be my ignorance
| mut meshes: ResMut<Assets<Mesh>>, | ||
| mut materials: ResMut<Assets<StandardMaterial>>, | ||
| ) { | ||
| let mut gizmo = GizmoAsset::new(); |
There was a problem hiding this comment.
Any reason why this cannot stay in this existing example? I know this PR basically moves and modifies this in the retained_gizmo’s example, but this code has a reason to be here: for documenting the differences in performance between immediate (system param) vs retained (component) gizmos, and giving an example that the 30,000 line static gizmo sphere is therefore better suited for retaining
There was a problem hiding this comment.
i guess it was just an aesthetic choice, it felt bolted-on in the existing example. i'm not married to the removal
|
|
||
| // When drawing a lot of static lines a Gizmo component can have | ||
| // far better performance than the Gizmos system parameter, | ||
| // but the system parameter will perform better for smaller lines that update often. |
There was a problem hiding this comment.
I know this comment is copy-pasted from the 3d example, but this makes me wonder if a retained gizmo makes sense here if the smaller lines are updating often with the spinning that happens, or does spin not happen often enough?
There was a problem hiding this comment.
yeah you're right, i just blindly copied this when i made the new example
| @@ -0,0 +1,161 @@ | |||
| //! This example demonstrates retained gizmos. | |||
There was a problem hiding this comment.
Since this example is user-facing and intended to inform the user, I think this doc comment could be expanded to tell
- How to make retained gizmos
- How their behavior differs from immediate gizmos
- Maybe a mention of the visibility culling
- The free camera plugin being included and the intent for why it’s included in this example.
I’d also like to see if there can be any more comments throughout the example in general about what the code is doing at a high level for a beginner audience where it makes sense, if you feel anything might be confusing
Here’s an initial suggestion for the doc comment of the example:
| //! This example demonstrates retained gizmos. | |
| //! This example demonstrates retained gizmos. | |
| //! | |
| //! Retained gizmos are created with a handle to a [`GizmoAsset`]. | |
| //! Compared to immediate gizmos (gizmos made with the `Gizmos` system parameter), | |
| //! retained gizmos are recommended for gizmos with a larger number of static lines. | |
| //! Retained gizmos also can undergo frustum culling. | |
| //! | |
| //! This example spawns more retained gizmos the longer that it is active, | |
| //! capping the number of gizmos at 25. The gizmos spin constantly | |
| //! and also randomly twinkle (via togging visibility) at random times. | |
| //! | |
| //! This example uses the free camera plugin for observing the scene. |
|
|
||
| let total = gizmos.iter().count(); | ||
|
|
||
| if total > 25 && rng.random_bool((total as f64 / 150.0).min(1.0)) { |
There was a problem hiding this comment.
I think 25 can be extracted as a const to make it more obvious this is an important magic number
Unfortunately I'm not either! I just followed existing code so I certainly am not confident I did everything 100% correctly
I was doing a lot of debug logging (which I removed before committing) to validate it, I wasn't sure of a good way to verify automatically though. Maybe I can add a system in the render world to count visible gizmos. I'm not too familiar with the best approach to integration test so many interacting systems but I'm willing to try stuff |
3 participants
Objective
Solution
Testing